Skip to content

[hist] Improve limits of THLimitsFinder. #19605

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 23, 2025
Merged

[hist] Improve limits of THLimitsFinder. #19605

merged 3 commits into from
Sep 23, 2025

Conversation

hageboeck
Copy link
Member

THLimitsFinder sometimes removes the first/last bin of an axis range. Here, it is ensured that the min and max of a buffered range is part of the axis range, and not removed by accident.
This fixes issues with TTree::Draw such as the one described in https://root-forum.cern.ch/t/bug-or-feature-in-ttree-draw/62862/

This supersedes #17689

@hageboeck hageboeck self-assigned this Aug 11, 2025
@hageboeck hageboeck added the skip ci Skip the full builds on the actions runners label Aug 11, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 11, 2025 •
edited
Loading

Test Results

    21 files      21 suites   3d 13h 22m 49s ⏱️
 3 661 tests  3 661 ✅ 0 💤 0 ❌
75 108 runs  75 108 ✅ 0 💤 0 ❌

Results for commit 7c7ad25.

♻️ This comment has been updated with latest results.

@hageboeck hageboeck mentioned this pull request Aug 11, 2025
Closed
2 tasks
ferdymercury
ferdymercury approved these changes Aug 11, 2025
View reviewed changes
Copy link
Collaborator

@ferdymercury ferdymercury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

I would propose this test:

TTree t;
Float_t x;
t.Branch("x", &x);
x = -999;
t.Fill();
x = 0;
t.Fill();
t.Draw("x");
auto h = (TH1*)gROOT->FindObject("htemp");
ASSERT_EQ(h->GetEntries(), h->GetEffectiveEntries());

@hageboeck hageboeck removed the skip ci Skip the full builds on the actions runners label Aug 13, 2025
@hageboeck hageboeck marked this pull request as ready for review August 13, 2025 15:06
@hageboeck
Copy link
Member Author

LGTM thanks!

I would propose this test:

TTree t;
Float_t x;
t.Branch("x", &x);
x = -999;
t.Fill();
x = 0;
t.Fill();
t.Draw("x");
auto h = (TH1*)gROOT->FindObject("htemp");
ASSERT_EQ(h->GetEntries(), h->GetEffectiveEntries());

@ferdymercury good test! It's added now.

ferdymercury
ferdymercury approved these changes Aug 13, 2025
View reviewed changes
Copy link
Collaborator

@ferdymercury ferdymercury left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just a nitpick at the end of DrawAutoBinning:

delete h;
delete gROOT->FindObject("c1");

@hageboeck
Copy link
Member Author

Thanks! Just a nitpick at the end of DrawAutoBinning:

delete h;
delete gROOT->FindObject("c1");

👍, updated for h. The canvas isn't created in batch mode, though, so that should be be unnecessary in this test.

@ferdymercury
Copy link
Collaborator

The canvas isn't created in batch mode, though, so that should be be unnecessary in this test.

Are you sure?

root -l -b
root [0] TTree t;
root [1] Float_t x;
root [2] t.Branch("x", &x);
root [3] x = -999;
root [4] t.Fill();
root [5] x = 0;
root [6] t.Fill();
root [7] t.Draw("x");
Info in <TCanvas::MakeDefCanvas>:  created default TCanvas with name c1
root [8] gROOT->FindObject("c1")
(TObject *) 0x5c740608b9c0
root [9] delete gROOT->FindObject("c1")
root [10] gROOT->FindObject("c1")
(TObject *) nullptr

@hageboeck
Copy link
Member Author

The test failures for the graphics are real. This will require more investigation.

ferdymercury
ferdymercury reviewed Aug 14, 2025
View reviewed changes
ferdymercury
ferdymercury reviewed Aug 14, 2025
View reviewed changes
@hageboeck hageboeck force-pushed the limits_finder branch 3 times, most recently from 6397529 to 1a81a93 Compare August 18, 2025 13:56
@hageboeck hageboeck requested a review from dpiparo as a code owner August 18, 2025 13:56
@hageboeck hageboeck added the clean build Ask CI to do non-incremental build on PR label Aug 19, 2025
ferdymercury
ferdymercury reviewed Aug 25, 2025
View reviewed changes
ferdymercury
ferdymercury reviewed Sep 8, 2025
View reviewed changes
hageboeck and others added 2 commits September 15, 2025 16:01
@hageboeck
[hist] Stabilise a test in stressHistogram.
3f5a174 
During the merge of histograms with automatic axis ranges, the axis
range was computed when the fill buffer of the first histogram is full.
In this case, this was about halfway during the merge.
The resulting histogram was compared to one whose axis range was
computed on the full statistics, leading to slightly different axes.
By chance, the differences were within the tolerance, but in the commit
to follow, the axis range will be extended slightly to make the maximum
of a distribution part of the valid range. This will make the test fail.

This commit stabilises the test by extending the fill buffer of the first
histogram like for all the others in the test. In this way, all axis ranges
are computed on the same data.
@hageboeck @ferdymercury
[hist] Ensure that maximum of a distribution is visible in histograms.
439bc5b 
- When asking THLimitsFinder to optimise axis limits, ask for a
  maximum slightly right of the actual maximum of the distribution.
  Otherwise, since the maximum may coincide with the maximum of the
  axis range, values may fall into the overflow.
- Use std::min / std::max to ensure that OptimizeLimits doesn't truncate
  any values.
- For symmetry reasons, also extend the range slightly to lower values.

This fixes the problem observed in
https://root-forum.cern.ch/t/bug-or-feature-in-ttree-draw/62862

Co-authored-by: ferdymercury <[email protected]>
guitargeek
guitargeek reviewed Sep 15, 2025
View reviewed changes
guitargeek
guitargeek approved these changes Sep 15, 2025
View reviewed changes
Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much!

@hageboeck @ferdymercury @guitargeek
Add two tests for auto-binning of TH1 and in TTree::Draw.
7c7ad25 
THLimitFinder tries to trim empty bins close to the end of an axis range,
but sometimes, the min/max was trimmed as well, so not all data would be
visible in the histogram.

Here, the case from the following post is tested:
https://root-forum.cern.ch/t/bug-or-feature-in-ttree-draw/

Co-authored-by: ferdymercury <[email protected]>
Co-authored-by: Jonas Rembser <[email protected]>
@hageboeck hageboeck merged commit 89511a2 into root-project:master Sep 23, 2025
68 of 72 checks passed
@hageboeck hageboeck deleted the limits_finder branch September 23, 2025 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guitargeek guitargeek guitargeek approved these changes

@pcanal pcanal Awaiting requested review from pcanal pcanal is a code owner

@lmoneta lmoneta Awaiting requested review from lmoneta lmoneta is a code owner

@dpiparo dpiparo Awaiting requested review from dpiparo dpiparo is a code owner

+1 more reviewer

@ferdymercury ferdymercury ferdymercury approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@hageboeck hageboeck

Labels
clean build Ask CI to do non-incremental build on PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hageboeck @ferdymercury @guitargeek